Skip to content

Allow to configure restrictedToMinimumLevel via LoggingLevelSwitch #241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

zvirja
Copy link

@zvirja zvirja commented May 23, 2025

While configuring a sink in Serilog, it's possible to pass both static LogEventLevel, but also the LoggingLevelSwitch to control the value dynamically. It gives a bit of flexibility. Unfortunately, as of now the config.WriteTo.Seq(..) API doesn't allow to pass the value.

In this PR I just introduce the parameter, so it's possible to use both static and dynamic value.

@nblumhardt
Copy link
Member

Thanks for sending this. I think the omission was deliberate, because of possible confusion with the controlLevelSwitch parameter accepted by the method. We should review this, as they're distinct, but there are some binary compatibility requirements around this method signature we'll also need to think through, so this could take a little while.

In the meantime, one possible workaround would be:

    .WriteTo.Conditional(
        evt => levelSwitch.IsEnabled(evt.Level),
        wt => wt.Seq(...))

@zvirja
Copy link
Author

zvirja commented May 26, 2025

@nblumhardt wow, thanks for the suggestion! it works perfectly fine for me.

Yet I think we should have the API I am suggesting in this PR. Primarily, to be on par with e.g. .ApplicationInsights() - they have both in their API. This way people would be less surprised and code more following POLA.

As for the breaking change - well, you know better your policies. But feels like it could be just enough to bump the major and have compilation-time compatibility. There is always as well a possibility to just introduce an overload. But you are smart people, you are aware of all of this 😉 So please take your time.

Thanks for the fast reply!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants